Skip to content

Render basic tables and detect them in rules#507

Open
masonium wants to merge 9 commits intodaisy:mainfrom
orchid-initiative:array_rule
Open

Render basic tables and detect them in rules#507
masonium wants to merge 9 commits intodaisy:mainfrom
orchid-initiative:array_rule

Conversation

@masonium
Copy link
Contributor

Addresses issue #283 .

This PR adds some internal functions to compute table dimensions (rows and columns) taking rowspans and colspans into account, and uses them to render tables with the :array intent as "table with m rows and n columns".

I'm not super happy with the current logic to detect that an unspecified mtable should have an :array intent. Right now, it assumes that any mtable that either renders its frame as solid or dashed or has an mrow or mtd that specifies a rowspan or colspan should be an :array. I think we want something more general, though.

@rileeki
Copy link

rileeki commented Feb 28, 2026

@Jeffrey-Kuan

I'm not super happy with the current logic to detect that an unspecified mtable should have an :array intent. Right now, it assumes that any mtable that either renders its frame as solid or dashed or has an mrow or mtd that specifies a rowspan or colspan should be an :array. I think we want something more general, though.

Any thoughts?

@rileeki
Copy link

rileeki commented Feb 28, 2026

Also, just a note: this PR currently fails one of the CI checks, but so does the main branch. @masonium submitted a separate PR (#506) to fix the test that's failing.

@NSoiffer
Copy link
Collaborator

NSoiffer commented Mar 1, 2026

Using frame, etc., is a good idea and will work for legacy MathML so we should keep it. However, in MathML core (which is the future), mtable are supposed to be styled with CSS so that info won't likely be around unless it is a local CSS style (style="...."). If you want, you could add a check for that. But people will likely use a class for that so there is no hope to get that info in general.

One option is to look at the parent of the table and see if has delimiters around it (parens, brackets, vertical bars, double vertical bars, ???).

Comments on the code:
speech.rs:

  • why add std::format::Debug and pub trait TreeOrString<'c, 'm:'c, T: Debug> : Debug {?

xpath-functions.rs:

  • why is struct CountTableDims pub?
  • at least when looking in github's PR files changed tab, the indenting is messed up making it hard to read the code.
  • the spanning column code only looks a colspan in the first row. If you are supporting colspan, shouldn't you get this right no matter where the colspan is located. It isn't easy. That's Handle rowspan/columnspan #33.
  • rowspan isn't handled at all. No need for CountTableRows if you're not handling these.

mtable.rs:

  • there aren't any tests that use rowspan or colspan. It is good that you have a unit test in xpath-functions.rs, but you need to test the actual speech also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants